Skip to content

[Neo4j] DtoRepo.delete #3220

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

[Neo4j] DtoRepo.delete #3220

wants to merge 2 commits into from

Conversation

CarsonF
Copy link
Member

@CarsonF CarsonF commented May 29, 2024

  • Add DtoRepo.delete to replace deleteNode
    This query is specific to the resource type of the class, which is safer (assumes less) than any type.
    It also matches the EdgeDB version so migration should be seamless.
  • Mask generic Neo4j errors just like with EdgeDB
    There shouldn't be any need to: 
    } catch (e) {
  ...
  throw new ServerException('Failed to do X');
}
    Now you can throw e or skip catch all together if there's no other logic.

CarsonF added 2 commits May 29, 2024 15:26
@CarsonF
Add DtoRepo.delete to replace deleteNode
d1111f8 
This query is specific to the resource type of the class.
It also matches the EdgeDB version so migration should be seamless
@CarsonF
Mask generic Neo4j errors just like with EdgeDB
a9bf1ad
@CarsonF CarsonF requested a review from bryanjnelson May 29, 2024 20:30
bryanjnelson
bryanjnelson approved these changes May 29, 2024
View reviewed changes
Copy link
Contributor

@bryanjnelson bryanjnelson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

@CarsonF CarsonF changed the title [Neo4j] DtoRepo.delete / Generic error masking [Neo4j] DtoRepo.delete May 29, 2024
@CarsonF
Copy link
Member Author

CarsonF commented May 29, 2024

Ah this doesn't work because it forces all subclasses to conform to this delete signature.
We still want subclasses to be able to change that as this is just a builder/helper class.
I'm not going to go through the effort of implementing the omit logic here, which is how we got around this on the EdgeDB side.

@CarsonF CarsonF closed this May 29, 2024
@CarsonF CarsonF deleted the neo4j/delete branch May 29, 2024 21:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bryanjnelson bryanjnelson bryanjnelson approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@CarsonF @bryanjnelson